[INF-5286] Write reconciliation functions that pave over inconsistencies in API responses#233
Open
martin-ably wants to merge 1 commit intomainfrom
Open
[INF-5286] Write reconciliation functions that pave over inconsistencies in API responses#233martin-ably wants to merge 1 commit intomainfrom
martin-ably wants to merge 1 commit intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 35 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (31)
WalkthroughAdds a reconciler to merge Terraform plan/state with Ably API responses, extensive unit tests for reconciliation logic, refactors many resource CRUD mappings to use the reconciler, introduces shared minimal HCL test helpers, and adds numerous minimal acceptance tests plus a small CI timeout tweak. Changes
Sequence Diagram(s)sequenceDiagram
participant Terraform as Terraform (plan/state)
participant Provider as Provider (reconciler)
participant API as Ably API
participant State as Terraform State
Terraform->>Provider: Create/Read/Update with plan/state
Provider->>API: Call Ably API (Create/Read/Update)
API-->>Provider: API response (resource fields)
Provider->>Provider: reconciler compares plan vs API (str/bool/int/slice/map)
Provider-->>State: Set reconciled Terraform types and diagnostics
State-->>Terraform: Apply/Plan result (includes computed values)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
martin-ably
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
surminus
reviewed
Apr 13, 2026
martin-ably
force-pushed
the
martin/qos/write-reconciliation-function
branch
from
5db2c4a to
4d0f9c7
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
martin-ably
force-pushed
the
martin/qos/write-reconciliation-function
branch
2 times, most recently
from
6955671 to
dbaf109
Compare
Collaborator
Author
|
I've tested this version against my account in production, and it's creating the apps correctly, as well as the keys. So if this change doesn't work in the main production account, then the account itself might be having some issues. |
surminus
reviewed
Apr 13, 2026
| // buildQueueState reconciles plan/state input with an API response. | ||
| func buildQueueState(rc *reconciler, input AblyQueue, api control.QueueResponse) AblyQueue { | ||
| return AblyQueue{ | ||
| AppID: rc.str("app_id", input.AppID, types.StringValue(api.AppID), false), |
Contributor
There was a problem hiding this comment.
Rather than have to pass in a boolean value for whether it's computed or not, I wonder whether we could have a buildState() method on the resource type itself, which could lookup the field? But perhaps maybe that's something for a future simplification.
Contributor
|
I think this looks fine, although not applied to all resources. Is the plan to fix the broken resources only in this PR? |
martin-ably
force-pushed
the
martin/qos/write-reconciliation-function
branch
from
dbaf109 to
9ccd17e
Compare
Collaborator
Author
|
I was originally just patching up the main offenders, but I've added the remaining reconciliation code for the rules. I've had to implement additional functions to handle maps, slices, and planning field accessors, as the rules have more finicky logic (as they're all variants over generics). |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/provider/reconcile.go (1)
196-199:⚠️ Potential issue | 🟠 MajorRedact API strings from reconciliation errors.
output.ValueString()can be a secret (key,secret_access_key, tokens, header values, etc.). Emitting it in diagnostics leaks remote data into provider logs.💡 Suggested fix
- return types.StringNull(), fmt.Errorf( - "reconcile %q: API returned %q but field was not set in config and is not computed", - field, output.ValueString(), - ) + return types.StringNull(), fmt.Errorf( + "reconcile %q: API returned a value but field was not set in config and is not computed", + field, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/reconcile.go` around lines 196 - 199, The reconciliation error is leaking API-returned secret strings by interpolating output.ValueString() into the error; change the error construction in the reconcile code to avoid including the raw output value and instead emit a redacted placeholder (e.g. "<redacted>" or indicate presence/length) while preserving field context and the fact it was unexpected; update the return that currently references output.ValueString() to use the redacted token and keep the rest of the message and types.StringNull() behavior unchanged so callers can still detect the reconcile failure without exposing secrets.internal/provider/resource_ably_namespace.go (1)
174-181:⚠️ Potential issue | 🟠 MajorPreserve batching/conflation values when the enabled flag is omitted.
If
api.BatchingEnabledorapi.ConflationEnabledisnil, these branches skip reconciliation entirely and leavebatching_interval,conflation_interval, andconflation_keyunset. A partial API response will therefore clear previously configured values and reintroduce drift on the exact payloads this reconciler is supposed to absorb. Treatnilas “field omitted” and only clear these attributes when the API explicitly returnsfalse.💡 Suggested fix
- if api.BatchingEnabled != nil && *api.BatchingEnabled { - ns.BatchingInterval = rc.int64val("batching_interval", input.BatchingInterval, optIntValue(api.BatchingInterval), true) - } - - if api.ConflationEnabled != nil && *api.ConflationEnabled { - ns.ConflationInterval = rc.int64val("conflation_interval", input.ConflationInterval, optIntValue(api.ConflationInterval), true) - ns.ConflationKey = rc.str("conflation_key", input.ConflationKey, optStringValue(api.ConflationKey), true) - } + switch { + case api.BatchingEnabled == nil: + ns.BatchingInterval = input.BatchingInterval + case *api.BatchingEnabled: + ns.BatchingInterval = rc.int64val("batching_interval", input.BatchingInterval, optIntValue(api.BatchingInterval), true) + default: + ns.BatchingInterval = types.Int64Null() + } + + switch { + case api.ConflationEnabled == nil: + ns.ConflationInterval = input.ConflationInterval + ns.ConflationKey = input.ConflationKey + case *api.ConflationEnabled: + ns.ConflationInterval = rc.int64val("conflation_interval", input.ConflationInterval, optIntValue(api.ConflationInterval), true) + ns.ConflationKey = rc.str("conflation_key", input.ConflationKey, optStringValue(api.ConflationKey), true) + default: + ns.ConflationInterval = types.Int64Null() + ns.ConflationKey = types.StringNull() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/resource_ably_namespace.go` around lines 174 - 181, The reconciler currently treats a nil enabled flag as "disabled" because it only sets batching/conflation when api.BatchingEnabled or api.ConflationEnabled is true, which causes omitted fields to be cleared; change the logic so nil means "omit/do nothing" and only act when the pointer is non-nil: for api.BatchingEnabled, if api.BatchingEnabled == nil do nothing, else if *api.BatchingEnabled set ns.BatchingInterval via rc.int64val(...), otherwise explicitly clear ns.BatchingInterval; similarly for api.ConflationEnabled, if nil do nothing, else if true set ns.ConflationInterval and ns.ConflationKey using rc.int64val/rc.str with input.BatchingInterval/input.ConflationInterval/input.ConflationKey and optIntValue/optStringValue, and if false explicitly clear ns.ConflationInterval and ns.ConflationKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/provider/resource_ably_app_test.go`:
- Around line 255-258: The test assertion for the default of the tls_only
attribute is inverted: update the TestCheckResourceAttr call in
resource_ably_app_test.go that references "ably_app.app0", "tls_only" to expect
"false" (matching the schema default in resource_ably_app.go for tls_only);
leave the other defaults (e.g., "status" and "apns_use_sandbox_endpoint")
unchanged.
In `@internal/provider/resource_ably_rule_pulsar_test.go`:
- Around line 110-118: The test config variable created via minimalRuleConfig
includes a JWT-looking literal in the Pulsar target's authentication.token;
replace that hardcoded JWT-like string with a clearly fake placeholder or
generated dummy (e.g., "fake-token-placeholder" or a call that generates a
random string) so scanners won't flag it and tests remain safe. Update the token
value inside the config passed to minimalRuleConfig (the pulsar target's
authentication.token) and ensure any assertions that depend on the token are
adjusted to expect the placeholder or generated value.
In `@internal/provider/rules.go`:
- Around line 342-364: When rc.reading is true and the API-reported
auth.AuthenticationMode differs from planAuth.AuthenticationMode, force the
inactive-mode plan inputs to null so rc.str won't echo stale values: detect the
mismatch after setting modeOutput and before calling rc.str, and for the
inactive branch set the corresponding planAuth fields (e.g.,
planAuth.AccessKeyId / planAuth.SecretAccessKey for AWSAuthModeAssumeRole, or
planAuth.RoleArn for AWSAuthModeCredentials) to types.StringNull() (or pass a
null-valued variable) so rc.str("target.authentication.*", ...) receives a null
plan for those inactive fields; keep using
modeOutput/keyOutput/secretOutput/arnOutput as you already compute them and
return AwsAuth with rc.str calls unchanged.
---
Duplicate comments:
In `@internal/provider/reconcile.go`:
- Around line 196-199: The reconciliation error is leaking API-returned secret
strings by interpolating output.ValueString() into the error; change the error
construction in the reconcile code to avoid including the raw output value and
instead emit a redacted placeholder (e.g. "<redacted>" or indicate
presence/length) while preserving field context and the fact it was unexpected;
update the return that currently references output.ValueString() to use the
redacted token and keep the rest of the message and types.StringNull() behavior
unchanged so callers can still detect the reconcile failure without exposing
secrets.
In `@internal/provider/resource_ably_namespace.go`:
- Around line 174-181: The reconciler currently treats a nil enabled flag as
"disabled" because it only sets batching/conflation when api.BatchingEnabled or
api.ConflationEnabled is true, which causes omitted fields to be cleared; change
the logic so nil means "omit/do nothing" and only act when the pointer is
non-nil: for api.BatchingEnabled, if api.BatchingEnabled == nil do nothing, else
if *api.BatchingEnabled set ns.BatchingInterval via rc.int64val(...), otherwise
explicitly clear ns.BatchingInterval; similarly for api.ConflationEnabled, if
nil do nothing, else if true set ns.ConflationInterval and ns.ConflationKey
using rc.int64val/rc.str with
input.BatchingInterval/input.ConflationInterval/input.ConflationKey and
optIntValue/optStringValue, and if false explicitly clear ns.ConflationInterval
and ns.ConflationKey.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1cb81c6-3ca1-4700-859a-3f8ef7127f32
📒 Files selected for processing (30)
.github/workflows/check.ymlinternal/provider/ingress_rules.gointernal/provider/provider_test.gointernal/provider/reconcile.gointernal/provider/reconcile_test.gointernal/provider/resource_ably_app.gointernal/provider/resource_ably_app_test.gointernal/provider/resource_ably_ingress_rule_mongo_test.gointernal/provider/resource_ably_ingress_rule_postgres_outbox_test.gointernal/provider/resource_ably_key.gointernal/provider/resource_ably_key_test.gointernal/provider/resource_ably_namespace.gointernal/provider/resource_ably_namespace_test.gointernal/provider/resource_ably_queue.gointernal/provider/resource_ably_queue_test.gointernal/provider/resource_ably_rule_amqp_external_test.gointernal/provider/resource_ably_rule_amqp_test.gointernal/provider/resource_ably_rule_azure_function_test.gointernal/provider/resource_ably_rule_http_cloudflare_worker_test.gointernal/provider/resource_ably_rule_http_google_cloud_function_test.gointernal/provider/resource_ably_rule_http_test.gointernal/provider/resource_ably_rule_ifttt_test.gointernal/provider/resource_ably_rule_kafka_test.gointernal/provider/resource_ably_rule_kinesis_test.gointernal/provider/resource_ably_rule_lambda_test.gointernal/provider/resource_ably_rule_pulsar_test.gointernal/provider/resource_ably_rule_sqs_test.gointernal/provider/resource_ably_rule_zapier_test.gointernal/provider/rules.gointernal/provider/rules_unit_test.go
✅ Files skipped from review due to trivial changes (4)
- .github/workflows/check.yml
- internal/provider/resource_ably_ingress_rule_mongo_test.go
- internal/provider/resource_ably_rule_amqp_test.go
- internal/provider/resource_ably_rule_ifttt_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- internal/provider/resource_ably_rule_http_test.go
- internal/provider/resource_ably_rule_http_cloudflare_worker_test.go
- internal/provider/resource_ably_rule_lambda_test.go
- internal/provider/resource_ably_queue_test.go
- internal/provider/resource_ably_rule_http_google_cloud_function_test.go
- internal/provider/resource_ably_rule_amqp_external_test.go
- internal/provider/resource_ably_rule_kafka_test.go
- internal/provider/resource_ably_rule_sqs_test.go
- internal/provider/resource_ably_rule_azure_function_test.go
- internal/provider/resource_ably_rule_zapier_test.go
- internal/provider/resource_ably_rule_kinesis_test.go
- internal/provider/resource_ably_namespace_test.go
- internal/provider/ingress_rules.go
- internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go
- internal/provider/resource_ably_app.go
- internal/provider/provider_test.go
…I responses This should result in the TF provider being a little more resistant to upstream field changes.
martin-ably
force-pushed
the
martin/qos/write-reconciliation-function
branch
from
9ccd17e to
787fffc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This should result in the TF provider being a little more resistant to upstream field changes.
Additionally, adds tests that check if any unexpected fields are returned when a minimal (required-only) config is sent.
Summary by CodeRabbit
Tests
Refactor
Chores